Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add flag to control CORS Origin header #504

Closed
wants to merge 2 commits into from
Closed

Conversation

sean9999
Copy link

@sean9999 sean9999 commented Sep 4, 2021

I added a flag to allow the user to set their own Access-Control-Allow-Origin header. I feel it's proper to be able to set this header because CORS was designed with this use-case in mind. You need to be able explicitly say what origin can access your API. Using a reverse-proxy felt kludgy, because the app already sets CORS headers. It is not offloading that responsibility. This resolves #135 and #386.

I developed this for personal use because I am running tusd in a Kubernetes environment, and because of the way k8s handles network routing via Ingress controller, setting the proper Origin header was infeasible (even with --behind-proxy turned on). Using a reverse proxy felt wrong because k8s already does a lot of network indirection.

I hope you find this useful

header.Set("Access-Control-Allow-Origin", origin)
header.Set("Vary", "Origin")
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Acconut Acconut changed the title Develop Add flag to control CORS Origin header Oct 15, 2021
@Acconut
Copy link
Member

Acconut commented Oct 15, 2021

Thank you very much for this PR, but I have one question: Is it not necessary to validate the the Origin header in the request matches config.CorsOrigin? If they are not equal, we could return an error. Or is this not necessary? Right now, I don't see such a test but only that config.CorsOrigin is always set as the header value.

@DeepDiver1975
Copy link
Contributor

: Is it not necessary to validate the the Origin header in the request matches config.CorsOrigin?

Right now, I don't see such a test but only that config.CorsOrigin is always set as the header value.

The most simplistic setup on apache/nginx is also just adding the origin with no further logic applied. I'd say this is okay to go for the very first iteration. refs https://ubiq.co/tech-blog/enable-cors-apache-web-server/ (I just grabbed to first hit on google search 🙈 )

@sean9999 @Acconut is there anything I can help out with to get this PR going? THX a lot

@patrickjahns
Copy link

Currently also experimenting with tusd in a kubernetes setup. Adding this functionality would gladly improve the setup.

@sean9999 @Acconut - is there anything we can do to move this PR forward?

@G2G2G2G
Copy link

G2G2G2G commented Aug 7, 2022

Yea adding this would be nice.. For now I use proxy_hide_header Access-Control-Allow-Origin; on nginx which disables their bad header.

@sean9999
Copy link
Author

Is it not necessary to validate the the Origin header in the request matches config.CorsOrigin? If they are not equal, we could return an error. Or is this not necessary? Right now, I don't see such a test but only that config.CorsOrigin is always set as the header value.

You're right. I checked the spec, and a CORS compliant server is supposed to refuse a connection or return an error in that case. I'll try adding that in. https://www.w3.org/TR/2020/SPSD-cors-20200602/#resource-requests

@Acconut
Copy link
Member

Acconut commented Aug 11, 2022

I'll try adding that in

Sounds good! After that, I think we can merge this :)

@DeepDiver1975
Copy link
Contributor

Took the freedom to rebase this PR -> #942

@Acconut
Copy link
Member

Acconut commented Jun 12, 2023

Is it not necessary to validate the the Origin header in the request matches config.CorsOrigin? If they are not equal, we could return an error. Or is this not necessary? Right now, I don't see such a test but only that config.CorsOrigin is always set as the header value.

You're right. I checked the spec, and a CORS compliant server is supposed to refuse a connection or return an error in that case. I'll try adding that in. https://www.w3.org/TR/2020/SPSD-cors-20200602/#resource-requests

For anyone interested. This would be the last point that needs to be done. If this check is implement, we can move towards merging such a feature :)

@DeepDiver1975
Copy link
Contributor

can be closed as well - merged as part of #987

@Acconut
Copy link
Member

Acconut commented Sep 6, 2023

This is superseeded by #997. Thank you for all your help!

@Acconut Acconut closed this Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support custom headers for CORS
5 participants